-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-3276 [pymongo] FLE 1.0 shared library #947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to test with the csfle library and retain the existing mongocryptd tests. There are a few options:
- create new EVG tasks that runs test_encryption with auto-loaded csfle.
- duplicate the tests programmatically, run one set without csfle and one set with csfle_path.
- do what we did in pymongocrypt's testing and run setup.py test twice.
I like option 2 the best since it makes it makes the EVG test logs easier to reason about without bloating the test matrix.
Edit: After talking offline we decided that option 1) is better.
test/test_encryption.py
Outdated
| class TestBypassSpawningMongocryptdProse(EncryptionIntegrationTest): | ||
| @unittest.skipIf( | ||
| os.environ.get("TEST_CSFLE"), | ||
| "this prose test does not work when CSFLE is " "on a system dynamic library search path.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/mongodb/specifications/blob/master/source/client-side-encryption/tests/README.rst#8-bypass-spawning-mongocryptd
The note at the top says that you cannot run this test unless you disable CSFLE. The only way to disable CSFLE is to both not specify csfle_path and make sure CSFLE is not on any search paths. When TEST_CSFLE is specified, CSFLE is definitely on the search path, so we have to skip.
ShaneHarvey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work so far!
.evergreen/run-tests.sh
Outdated
| if [ -z $TEST_CSFLE ]; then | ||
| echo "CSFLE not being tested" | ||
| else | ||
| git clone git@github.com:mongodb-labs/drivers-evergreen-tools.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drivers-evergreen-tools is already cloned into $DRIVERS_TOOLS, eg:
$PYTHON $DRIVERS_TOOLS/.evergreen/mongodl.py ...
test/test_encryption.py
Outdated
| @unittest.skipUnless(os.environ.get("TEST_CSFLE"), "csfle is not installed") | ||
| def test_csfle(self): | ||
| # Test that we can pick up csfle automatically | ||
| MongoClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to close this client to clean up pymongocrypt resources.
| tasks: *encryption-server-versions | ||
| display_name: "Encryption ${platform} ${auth} ${ssl} ${encryption}" | ||
| tasks: &encryption-server-versions | ||
| - ".latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add 6.0 and rapid.
| - ".5.0" | ||
| - ".4.4" | ||
| - ".4.2" | ||
| - ".4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we don't want to test CSFLE with <6.0 we should use the "rules" field to ensure we're only testing classic fle with 4.0+ and shared lib fle with 6.0+: https://github.com/evergreen-ci/evergreen/wiki/Project-Configuration-Files#the-rules-field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a rules field but it does not seem to be removing the tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to a longstanding bug in EVG, the if condition needs to have all matrix axis in it:
rules:
- if:
platform: "*"
auth: "*"
ssl: "*"
encryption: [ "encryption_with_csfle" ]
then:...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now working for mac os, but not for the other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other ones have different axis.
| vars: | ||
| VERSION: "6.0" | ||
| TOPOLOGY: "sharded_cluster" | ||
| - func: "run tests" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already added 6.0 testing a while back. Why is this showing up here? Can we git merge master and fix the merge conflicts?
| - ".5.0" | ||
| - ".4.4" | ||
| - ".4.2" | ||
| - ".4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other ones have different axis.
ShaneHarvey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, waiting for this patch to succeed testing the shared library on all server versions on all platforms: https://spruce.mongodb.com/version/628ff72b56234342524f04d3
No description provided.